Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make the sizes of the members of enum Node more uniform #6545

Merged
merged 2 commits into from Sep 24, 2018
Merged

Make the sizes of the members of enum Node more uniform #6545

merged 2 commits into from Sep 24, 2018

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Sep 23, 2018 •
edited

Problem

The clippy shard is currently failing with a notice that there is a large variance between the sizes of the members of the Node enum. It refers to https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#large_enum_variant

According to mem::size_of, the sizes are currently:

  DigestFile(DigestFile),              // 32
  ExecuteProcess(ExecuteProcess),      // 200
  ReadLink(ReadLink),                  // 24
  Scandir(Scandir),                    // 24
  Select(Select),                      // 232
  Snapshot(Snapshot),                  // 16
  Task(Task),                          // 192

Solution

Box the three largest members to appease clippy.

Result

No appreciable difference in performance; happy clippy.

@stuhood stuhood changed the title WIP: Make the sizes of the members of enum Node more uniform Make the sizes of the members of enum Node more uniform Sep 23, 2018
illicitonion
illicitonion approved these changes Sep 24, 2018
View reviewed changes
Copy link
Contributor

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, thanks :) Probably significantly decreases memory consumption...

If you're just doing this to appease clippy, and would rather disable the lint, I'd be fine with that too, but I suspect the memory savings will actually be non-trivial.

src/rust/engine/src/nodes.rs
@@ -841,12 +841,12 @@ impl NodeTracer<NodeKey> for Tracer {
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub enum NodeKey {
DigestFile(DigestFile),
ExecuteProcess(ExecuteProcess),
ExecuteProcess(Box<ExecuteProcess>),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth adding a comment on the enum about why some things are boxed

@stuhood stuhood merged commit 1924650 into pantsbuild:master Sep 24, 2018
@stuhood stuhood deleted the stuhood/clippy-in-a-box branch September 24, 2018 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@illicitonion illicitonion illicitonion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@stuhood @illicitonion